doc: Spec of finality vote submission #120

gitferry · 2024-11-07T09:36:59Z

No description provided.

Lazar955

Concise write up!

Lazar955 · 2024-11-07T09:54:19Z

docs/send-finality-vote.md

+
+#### Batch submission
+
+A batch submission mechanism is needed to deal with cases where:


I would maybe note that there is a synchronization mechanism between loops so that we don't submit from fast sync loop and regular one

In the current code, all the submission is done in the submission loop. So there's no race condition. The fast sync loop is for sending the lagging signal. If lagging is detected, the sumission loop will do fast sync. See

finality-provider/finality-provider/service/fp_instance.go

Line 254 in 3f8aa01

case targetBlock := <-fp.laggingTargetChan:

.

Nevertheless, in the spec, we use batch submission other than fast sync and we plan to remove the latter completely.

bap2pecs · 2024-11-08T06:30:37Z

docs/send-finality-vote.md

+
+## Internal State
+
+The finality provider maintains two critical persistent states:


i think it's simpler to just keep one persistent state in DB (i.e. last processed height)

last voted height can be easily retrieved from remote

actually do you think we can get rid of both from DB and only keeps one in-memory state (i.e. fp.nextHeightToProcess)?

I feel it's doable

Good point. I think we maybe can get rid of lastVotedHeight but why do you say it's easy to retrieve lastVotedHeight from the remote? It seems that we need to save this info in the finality provider on Babylon

actually do you think we can get rid of both from DB and only keeps one in-memory state (i.e. fp.nextHeightToProcess)?

if we get rid of fp.nextHeightToProcess, we will waste some resources processing blocks that have already been processed, won't we?

but why do you say it's easy to retrieve lastVotedHeight from the remote? It seems that we need to save this info in the finality provider on Babylon

b/c if we store it in a local DB, there will be two source of truth. for the OP consumer FP, we can easily add a query in the CosmWasm contract for it e.g. getFpLastVotedHeight

if we get rid of fp.nextHeightToProcess, we will waste some resources processing blocks that have already been processed, won't we?

no I mean to get rid of the local DB states but keep nextHeightToProcess

how it works is: at startup, the FP will get everything it needs from the Babylon Chain and/or the CosmWasm contract to set nextHeightToProcess

but nextHeightToProcess is not a DB state, it's an in-memory field value of the FinalityProvider object

presisting lastProcessedHeight means engineers will need to have it in their mental model to make sure this data is updated and think about edge cases where it can go out of sync. and also need to think about when and where to compare with lastVotedHeight and what the results imply.

that's why I think it's not worth it if the edge case discussed will almost never gonna happen in the real world. and even if it happens, the impact is negligible.

but I don't have a strong preference either. will leave it to your call

Makes sense. Will bring it to the team to make the decision. Appreciate the discussion!

After a second thought, I feel that we can get rid of lastProcessedHeight completely (leaving unharmful edge case open) but we probably need to keep the lastVotedHeight both in db and memory. If we only have it in memory, there would be an attack I can think of:

Consider a fp is crashed right after sending a vote for height 100. After restart, it syncs with the remote and initialize the last voted height. However, the last voted height of the fp from the remote could be still 99 due to the late excution of the block. Therefore, it is possible for the fp to send another vote for 100. This puts the fp under the risk of being slashed due to signing the same height twice.

However, the last voted height of the fp from the remote could be still 99 due to the late excution of the block.

do u mean the babylon chain didn't execute the block containing the vote in time?

but as long as the FP signs the same block at height 100, it won't be slashed.

the assumption is the FP is connected to a trusted RPC that won't send forks to it.

but i guess it's probably safer to have some redundancy considering FP is crucial

Yep, I don't think it's safe to assume that FP is connected to a trusted RPC as we can not control it. The bottom line is that fp should not sign twice for the same height.

bap2pecs · 2024-11-08T06:51:21Z

docs/send-finality-vote.md

+2. If no finalized blocks exist:
+   - Start from `finalityActivationHeight`


there is a problem here.

imagine there is no finalized block, but this FP already voted for the first activated block and it's just waiting for the rest FPs to vote so the block can get finalized

starting at finalityActivationHeight means a duplicate vote

Good point. See updates in ab8913a

parketh · 2024-11-08T09:39:22Z

docs/send-finality-vote.md

+Tracks the most recent height for which a finality vote was successfully
+submitted. This prevents duplicate voting on previously voted heights.
+
+### Last processed height


what's the difference between last voted height and last processed height?

last processed height additionally covers the case where the fp does not have voting power

KonradStaniec · 2024-11-13T08:46:46Z

docs/send-finality-vote.md

+4. Synchronize local state:
+   - Ensure local `lastVotedHeight` and `lastProcessedHeight` ≥ `lastVotedHeightRemote`
+5. Begin processing at:
+   - `max(lastProcessedHeight + 1, lastFinalizedHeight + 1, finalityActivationHeight)`


two things:

lastProcessedHeight is not initialised anywhere. I assume it comes from persistent state. Lets add the step of retrievieng it from database

in some consumer chains, it maybe possible to vote and receive reward even after finality is reached, so starting from lastFinalizedHeight + 1 may leave some rewards on the table. I would maybe leave it as comment here.

lastProcessedHeight is not initialised anywhere. I assume it comes from persistent state. Lets add the step of retrievieng it from database

In the discussion here I think we could probably be able to remove lastProcessedHeight as it is not critical to security.

in some consumer chains, it maybe possible to vote and receive reward even after finality is reached, so starting from lastFinalizedHeight + 1 may leave some rewards on the table. I would maybe leave it as comment here.

Good point. Fixed in 8732f15

KonradStaniec · 2024-11-13T08:49:17Z

docs/send-finality-vote.md

+### Normal Submission Loop
+
+After the finality provider is bootstraped, it continuously monitors for
+new blocks. For each new block, it performs these validation checks:


Do you think it is worth mentioning that finality provider receives block from trusted full node of consumer chain (or at least light client ) ?

Good point. Fixed in 8732f15

bap2pecs · 2024-11-16T08:06:46Z

how should we handle these two cases:

pub rand is lagging behind a lot to the tip height
FP is currently in jail or just get out of jail

gitferry · 2024-11-17T14:58:17Z

pub rand is lagging behind a lot to the tip height

When a fp is started, a sperate routine for committing public randomness should be started. This part if not the cope of this spec.

FP is currently in jail or just get out of jail

In this case, the operator should get the fp unjailed first. Will have a separate doc for this

bap2pecs · 2024-11-18T00:02:02Z

When a fp is started, a sperate routine for committing public randomness should be started. This part if not the cope of this spec.

but the vote submission routine has to wait for the PR is timestamped, so it's still relevant?

gitferry · 2024-11-18T01:46:46Z

but the vote submission routine has to wait for the PR is timestamped, so it's still relevant?

If the PR for a block is not timestamped, then the voting power is 0 for good. Even though the PR is timestamped later, the result won't change

bap2pecs · 2024-11-18T01:53:42Z

If the PR for a block is not timestamped, then the voting power is 0 for good

I am not aware of this. shouldn't vp only be determined by the btc delegations?

gitferry · 2024-11-18T02:47:29Z

I am not aware of this. shouldn't vp only be determined by the btc delegations?

This condition was added ffter integration with BTC-timestamping, see the figure here: a fp will only have voting power if (1) it has top N delegations, and (2) it has timestamped randomness for the block

bap2pecs · 2024-11-18T02:50:55Z

(1) it has top N delegations

what does this mean?

bap2pecs · 2024-11-18T02:57:38Z

This condition was added ffter integration with BTC-timestamping, see the figure here: a fp will only have voting power if (1) it has top N delegations, and (2) it has timestamped randomness for the block

this is good to know. but the important implication is when we have PR lagging for whatever reason (the 4 scenarios as I described in #131), it's possible that the entire consumer chain's total VP becomes zero for some range of blocks. so FPs will skip voting for those blocks. I guess there is nothing much we can do in such an extreme case, but it is worth pointing out. cc @SebastianElvis

gitferry · 2024-11-18T03:13:34Z

this is good to know. but the important implication is when we have PR lagging for whatever reason (the 4 scenarios as I described in #131), it's possible that the entire consumer chain's total VP becomes zero for some range of blocks. so FPs will skip voting for those blocks. I guess there is nothing much we can do in such an extreme case, but it is worth pointing out. cc @SebastianElvis

Yes, this is an extreme case but valid. In this case, we would have a period where no blocks will be finalized until at least one fp has voting power and sends the vote

The `lastProcessedHeight` was introduced for fast catch up blocks for which the fp does not have voting power. However, this case rarely happens and is unharmful. More discussions can be found [here](#120 (comment)).

gitferry added 3 commits November 7, 2024 17:35

init spec

73a9e54

minor

45ad883

changelog

3d90dd0

gitferry marked this pull request as ready for review November 7, 2024 09:38

gitferry requested review from vitsalis, SebastianElvis, KonradStaniec, Lazar955, RafilxTenfen, samricotta and bap2pecs November 7, 2024 09:39

Lazar955 approved these changes Nov 7, 2024

View reviewed changes

bap2pecs reviewed Nov 8, 2024

View reviewed changes

parketh reviewed Nov 8, 2024

View reviewed changes

gitferry added 2 commits November 11, 2024 17:38

change initial height

ab8913a

add useful links

b098562

KonradStaniec reviewed Nov 13, 2024

View reviewed changes

gitferry added 2 commits November 17, 2024 22:14

Merge branch 'main' into gai/spec-finality-vote

988d610

pr comments

8732f15

gitferry requested a review from KonradStaniec November 18, 2024 13:11

KonradStaniec approved these changes Nov 19, 2024

View reviewed changes

gitferry merged commit a5f022f into main Nov 19, 2024
12 checks passed

gitferry deleted the gai/spec-finality-vote branch November 19, 2024 02:08

This was referenced Nov 20, 2024

Remove LastProcessedHeight from internal state #140

Closed

chore: Remove last processed height #167

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: Spec of finality vote submission #120

doc: Spec of finality vote submission #120

gitferry commented Nov 7, 2024

Lazar955 left a comment

Lazar955 Nov 7, 2024

gitferry Nov 7, 2024

bap2pecs Nov 8, 2024 •

edited

Loading

bap2pecs Nov 8, 2024

gitferry Nov 11, 2024

bap2pecs Nov 11, 2024

bap2pecs Nov 11, 2024

bap2pecs Nov 11, 2024 •

edited

Loading

gitferry Nov 12, 2024

gitferry Nov 12, 2024

bap2pecs Nov 12, 2024

gitferry Nov 13, 2024

bap2pecs Nov 8, 2024 •

edited

Loading

gitferry Nov 11, 2024

parketh Nov 8, 2024

gitferry Nov 11, 2024 •

edited

Loading

KonradStaniec Nov 13, 2024

gitferry Nov 17, 2024

KonradStaniec Nov 13, 2024

gitferry Nov 17, 2024

bap2pecs commented Nov 16, 2024

gitferry commented Nov 17, 2024

bap2pecs commented Nov 18, 2024 •

edited

Loading

gitferry commented Nov 18, 2024

bap2pecs commented Nov 18, 2024

gitferry commented Nov 18, 2024

bap2pecs commented Nov 18, 2024

bap2pecs commented Nov 18, 2024

gitferry commented Nov 18, 2024


		#### Batch submission

		A batch submission mechanism is needed to deal with cases where:


		## Internal State

		The finality provider maintains two critical persistent states:

		2. If no finalized blocks exist:
		- Start from `finalityActivationHeight`

doc: Spec of finality vote submission #120

doc: Spec of finality vote submission #120

Conversation

gitferry commented Nov 7, 2024

Lazar955 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bap2pecs Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bap2pecs Nov 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bap2pecs Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gitferry Nov 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bap2pecs commented Nov 16, 2024

gitferry commented Nov 17, 2024

bap2pecs commented Nov 18, 2024 • edited Loading

gitferry commented Nov 18, 2024

bap2pecs commented Nov 18, 2024

gitferry commented Nov 18, 2024

bap2pecs commented Nov 18, 2024

bap2pecs commented Nov 18, 2024

gitferry commented Nov 18, 2024

bap2pecs Nov 8, 2024 •

edited

Loading

bap2pecs Nov 11, 2024 •

edited

Loading

bap2pecs Nov 8, 2024 •

edited

Loading

gitferry Nov 11, 2024 •

edited

Loading

bap2pecs commented Nov 18, 2024 •

edited

Loading